Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: custom sheet #2677

Merged
merged 7 commits into from
Mar 10, 2022
Merged

chore: custom sheet #2677

merged 7 commits into from
Mar 10, 2022

Conversation

Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Mar 9, 2022

What: change new StyleSheet to new cache.sheet.constructor

Why: close #2675

How:

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2022

🦋 Changeset detected

Latest commit: ea74c7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@emotion/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Jack-Works
Copy link
Contributor Author

hi! I cannot run jest -u to update the snapshot cause it complains

 FAIL  packages/react/__tests__/custom-cache.js
  ● Test suite failed to run

    SyntaxError: C:\Users\Jack\Workspace\emotion\packages\react\__tests__\custom-cache.js: Support for the experimental 
syntax 'jsx' isn't currently enabled (27:7):

      25 |   expect(
      26 |     render(
    > 27 |       <CacheProvider value={cache}>
         |       ^
      28 |         <div css={{ display: 'flex', color: 'blue' }} />
      29 |       </CacheProvider>
      30 |     )

    Add @babel/preset-react (https://git.io/JfeDR) to the 'presets' section of your Babel config to enable transformation.
    If you want to leave it as-is, add @babel/plugin-syntax-jsx (https://git.io/vb4yA) to the 'plugins' section to enable parsing.

      at Parser._raise (node_modules/jest-cli/node_modules/@babel/parser/src/parser/error.js:147:45)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 9, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ea74c7a:

Sandbox Source
Emotion Configuration

Comment on lines 58 to 62
if (Reflect.get(options.container, 'sheet')) {
return new GlobalSheet(options)
} else {
Reflect.set(options.container, 'sheet', this)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder - any particular reason why Reflect is used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To bypass the flow type check. sheet is not a property on the HTMLElement

@Andarist
Copy link
Member

Andarist commented Mar 9, 2022

hi! I cannot run jest -u to update the snapshot cause it complains

That is somewhat weird but don't worry - I will update them before merging

@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #2677 (4b790e4) into main (4266aa0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4b790e4 differs from pull request most recent head ea74c7a. Consider uploading reports for the commit ea74c7a to get more accurate results

Impacted Files Coverage Δ
packages/react/src/global.js 98.24% <100.00%> (ø)

@Andarist
Copy link
Member

Andarist commented Mar 9, 2022

I've wanted to push out changes to this branch - but it seems like I don't have sufficient permissions to push to your fork.

You can check how to allow this for future PRs here - https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork . At the moment, you can try applying this patch:

diff --git a/packages/react/__tests__/custom-cache.js b/packages/react/__tests__/custom-cache.js
index cab71fc3..86f6d89f 100644
--- a/packages/react/__tests__/custom-cache.js
+++ b/packages/react/__tests__/custom-cache.js
@@ -1,10 +1,11 @@
 // @flow
 /** @jsx jsx */
-import 'test-utils/next-env'
 import createCache from '@emotion/cache'
-import { jsx, CacheProvider, Global } from '@emotion/react'
+import { CacheProvider, Global, jsx } from '@emotion/react'
 import { StyleSheet } from '@emotion/sheet'
 import renderer from 'react-test-renderer'
+import { safeQuerySelector } from 'test-utils'
+import 'test-utils/next-env'
 
 function stylisPlugin(element) {
   if (element.type === 'decl' && element.value.startsWith('color:')) {
@@ -16,6 +17,11 @@ function render(ele) {
   return renderer.create(ele).toJSON()
 }
 
+beforeEach(() => {
+  safeQuerySelector('head').innerHTML = ''
+  safeQuerySelector('body').innerHTML = ''
+})
+
 test('with custom plugins', () => {
   let cache = createCache({
     key: 'custom-plugins',
@@ -40,40 +46,42 @@ test('with custom plugins', () => {
   `)
 })
 
-test('with custom sheet', () => {
+test('Global should "inherit" sheet class from the cache', () => {
   // https://github.com/emotion-js/emotion/issues/2675
   let cache = createCache({
     key: 'test',
     speedy: false
   })
-
-  class GlobalSheet extends StyleSheet {
-    insert(rule) {
-      return super.insert('/** global */' + rule)
-    }
-  }
   class MySheet extends StyleSheet {
-    constructor(options) {
-      super(options)
-      if (Reflect.get(options.container, 'sheet')) {
-        return new GlobalSheet(options)
-      } else {
-        Reflect.set(options.container, 'sheet', this)
-      }
-    }
-
     insert(rule) {
       super.insert(`/** ${this.key} */${rule}`)
     }
   }
   cache.sheet = new MySheet({ key: 'test', container: document.head })
 
-  expect(
-    render(
-      <CacheProvider value={cache}>
-        <div css={{ display: 'flex', color: 'blue' }} />
-        <Global styles={{ body: { width: '0' } }} />
-      </CacheProvider>
-    )
-  ).toMatchInlineSnapshot()
+  render(
+    <CacheProvider value={cache}>
+      <div css={{ color: 'hotpink' }} />
+      <Global styles={{ body: { width: '0' } }} />
+    </CacheProvider>
+  )
+
+  expect(safeQuerySelector('head')).toMatchInlineSnapshot(`
+    <head>
+      <style
+        data-emotion="test-global"
+        data-s=""
+      >
+        
+        /** test-global */body{width:0;}
+      </style>
+      <style
+        data-emotion="test"
+        data-s=""
+      >
+        
+        /** test */.test-1lrxbo5{color:hotpink;}
+      </style>
+    </head>
+  `)
 })

@Andarist Andarist merged commit ff3cb16 into emotion-js:main Mar 10, 2022
@Andarist
Copy link
Member

hi! I cannot run jest -u to update the snapshot cause it complains

That was weird and I've also experienced that. The problem is when you are using .toMatchInlineSnapshot() in this file - Jest tries to automatically parse the file and insert the generated snapshot as the argument to this call and it fails in the process. I didn't ever have this problem in this repo and I'm not sure if we have changed something in our configs recently or what. I'm pretty sure that I was using .toMatchInlineSnapshot() pretty extensively here in the past.

I didn't solve this problem, I just have provided a dummy value as a snapshot manually and then constructed the correct value manually based on the CLI report. I'll have to look into this later but that's a completely separate issue from this PR.

key: 'test',
speedy: false
})
class MySheet extends StyleSheet {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that you were constructing different sheet classes in the constructor of your custom implementation but perhaps that's not really needed. You can easily differentiate hashed/main and global sheets by just checking the options.key.endsWith('-global'). Based on that you can just assign some private property and branch the logic in the insert method. This way you only have to write a single StyleSheet class which might simplify your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I need to get the un-global version of the sheet so that's the easiest way, any way, thanks!

@github-actions github-actions bot mentioned this pull request Mar 10, 2022
@Jack-Works
Copy link
Contributor Author

That was weird and I've also experienced that.

Maybe because I'm using Windows? 🤔

@Jack-Works Jack-Works deleted the custom-sheet branch March 10, 2022 11:36
@Andarist
Copy link
Member

I have meant that the same thing has happened to me - so it was definitely not specific to your machine etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom StyleSheet
2 participants